Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in StateMap::get_mut #343

Merged
merged 2 commits into from
Sep 28, 2023
Merged

Fix bug in StateMap::get_mut #343

merged 2 commits into from
Sep 28, 2023

Conversation

Bargsteen
Copy link
Contributor

Purpose

This PR fixes an issue in StateMap::get_mut which allowed users to get inconsistent views of the data. There is an example below.

When using StateMap::get_mut a value is loaded from the persistent storage into memory. The in-memory value can then be mutated and it will be written to the persistent storage once the value is dropped (via the Drop implementation on StateRefMut).

If you are able to look up multiple mutable values from the state at once, you can get inconsistent views of the data.
Here is an example:
image
token_count has the in-memory value x + 2 and token_count has the value x as it is freshly loaded from persistence.

For this reason, we use lifetimes to ensure that you cannot have multiple mutable references to the state at the same time.
Or, at least that was what we meant to do!
We accidentally gave an immutable reference of self which exactly allows the example above to run instead of being caught by the borrow-checker.

Changes

  • Add three letters: mut (and a space)
  • Add proper tests using testbuild to ensure that it won't occur again

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

See the changelog for more details.
Copy link
Contributor

@abizjak abizjak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

@Bargsteen Bargsteen merged commit f6160eb into main Sep 28, 2023
133 checks passed
@Bargsteen Bargsteen deleted the kb/fix-get-mut branch September 28, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants